Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make Server::start infallible and add fn builder() #1137

Merged
merged 4 commits into from
May 28, 2023
Merged

make Server::start infallible and add fn builder() #1137

merged 4 commits into from
May 28, 2023

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented May 26, 2023 •
edited
Loading

Server::start was infallible to let's change the API to reflect that.

@niklasad1 niklasad1 requested a review from a team as a code owner May 26, 2023 14:04
@niklasad1 niklasad1 added the breaking change Breaking change in the public APIs label May 26, 2023
jsdw
jsdw approved these changes May 26, 2023
View reviewed changes
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The only reason to keep the result would be if we thought it might become fallible again, but best not to anticipate such things anyway :)

@niklasad1
Copy link
Member Author

niklasad1 commented May 26, 2023 •
edited
Loading

Yeah, I'm quite tempted of removing the entire start API then just start the server from the Builder but it may useful to manage when the server is started, not sure.

@jsdw
Copy link
Collaborator

jsdw commented May 26, 2023 •
edited
Loading

Having a guide gander, it looks like you may as well keep a separate thing to start it offhand; I tend to feel that breaking functionality apart like this is a good thing (and then providing higher level stuff to do the common cases where applicable)

(I'd probably add a Server::builder() method though as a random thought because that might be a little cleaner than importing a separate Builder type to use to build it, and if I'm looking to make a thing I usually don't think to look for a separate type to construct it)

@niklasad1 niklasad1 changed the title refactor(server): make Server::start infallible refactor(server): make Server::start infallible and add fn builder() May 28, 2023
@niklasad1 niklasad1 merged commit e81bdae into master May 28, 2023
@niklasad1 niklasad1 deleted the na-server-api-remove-needless-result branch May 28, 2023 10:32
@niklasad1
Copy link
Member Author

I will remove the start API in another PR.

It's quite messy to migrate all tests.

@niklasad1 niklasad1 changed the title refactor(server): make Server::start infallible and add fn builder() make Server::start infallible and add fn builder() May 28, 2023
@lexnv lexnv mentioned this pull request Jul 20, 2023
Merged
come-maiz added a commit to come-maiz/helios that referenced this pull request Sep 12, 2024
@come-maiz
jsonrpsee Server::start is infallible according to:
c675cc0 
paritytech/jsonrpsee#1137

This change allows to compile with jsonrpsee v0.19.0
ncitron pushed a commit to a16z/helios that referenced this pull request Sep 26, 2024
@come-maiz
feat: linux riscv64gc version (#374)
b9bc6bf 
* Update jsonrpsee to 0.18.0

* Update jsonrpsee ErrorObject

After v0.18.0, jsonrpsee::core::Error was removed.

These are just naive changes to get it to compile.

See: https://github.com/paritytech/jsonrpsee/releases/tag/v0.18.0

* Update jsonrpsee to 0.19.0

* jsonrpsee Server::start is infallible according to:
paritytech/jsonrpsee#1137

This change allows to compile with jsonrpsee v0.19.0

* Release the linux riscv64gc version

Depends on #372.

Part of #370.

* Install risc-v version with heliosup.

* cargo fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsdw jsdw jsdw approved these changes

@lexnv lexnv lexnv approved these changes

@Mmagkooo Mmagkooo Mmagkooo approved these changes

Assignees
No one assigned
Labels
breaking change Breaking change in the public APIs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@niklasad1 @jsdw @lexnv @Mmagkooo